Conversation
pjain1
changed the title
| // RewriteQueryForRollupTest exposes rewriteQueryForRollup for integration tests. This is to prevent cyclic dependency error. | ||
| func (e *Executor) RewriteQueryForRollupTest(ctx context.Context, qry *metricsview.Query) (string, bool, error) { | ||
| spec, err := e.rewriteQueryForRollup(ctx, qry) |
There was a problem hiding this comment.
It seems a little weird that "integration" tests need to access an internal function.
It would be nice if they could hit the real Query code path instead. Since the problem is to find out what table it uses, did you consider finding some way of accessing that? Some quick ideas:
- Tracking metadata about the latest query and returning it with e.g.
e.LatestQueryTable() - Recording the trace in-memory and accessing the rollup events
| // A coarser grain is derivable from a finer grain only if they are on the | ||
| // same branch (or one is sub-day/day and the other is on a branch rooted at day). | ||
| // For example, month is derivable from day, but not from week. | ||
| func GrainDerivableFrom(queryGrain, rollupGrain runtimev1.TimeGrain) bool { |
There was a problem hiding this comment.
Is this relevant for external callers of the metricsview package? It seems like an implementation detail only relevant to the rollup executor. If so, consider moving into the executor package as internal functions, or into a separate package with time/rollup utils.
| // time_grain is required | ||
| if rollup.TimeGrain == runtimev1.TimeGrain_TIME_GRAIN_UNSPECIFIED { | ||
| res.OtherErrs = append(res.OtherErrs, fmt.Errorf("rollup[%d]: time_grain is required", i)) | ||
| continue | ||
| } |
There was a problem hiding this comment.
nit: maybe also validate that it is greater than or equal to the smallest_time_grain
| res.OtherErrs = append(res.OtherErrs, fmt.Errorf("rollup[%d]: dimension %q not found in metrics view", i, dimName)) | ||
| continue | ||
| } | ||
| if err := e.validateDimension(ctx, t, d, cols); err != nil { |
There was a problem hiding this comment.
Would it be easy/possible to refactor validateAllDimensionsAndMeasures so it can be used here? I remember we added it since sequential dimension/measure validation got very slow for metrics views with many dimensions/measures.
| if len(mv.ValidSpec.Rollups) > 0 && mv.ValidSpec.TimeDimension != "" { | ||
| var timeDim string | ||
| if q.TimeRange != nil { | ||
| timeDim = q.TimeRange.TimeDimension | ||
| } | ||
| tsRes, err := ResolveTimestampResult(ctx, rt, instanceID, q.MetricsViewName, timeDim, q.SecurityClaims, priority) |
There was a problem hiding this comment.
Not sure if it really matters, but is this still needed when timeDim != "" && mv.ValidSpec.TimeDimension != timeDim?
| "go.opentelemetry.io/otel/trace" | ||
| ) | ||
|
|
||
| const defaultTimestampsCacheTTL = 5 * time.Minute |
There was a problem hiding this comment.
Do you think it would make sense to move this to InstanceConfig so it can be changed dynamically if needed?
2 participants
Checklist: